fix(oauth): The dynamic client registration should be optional #463
Conversation
github-actions
bot
added
T-core
| authorization_endpoint: create_endpoint("authorize"), | ||
| token_endpoint: create_endpoint("token"), | ||
| registration_endpoint: create_endpoint("register"), | ||
| registration_endpoint: None, |
There was a problem hiding this comment.
Given that supporting DCR is a SHOULD in the MCP spec, If we cannot get metadata at all and we construct these defaults, should we continue to populate a default uri here?
There was a problem hiding this comment.
It aimed to achieve better compatibility with the RFC 8414 protocol, according to the rfc 8414 , it should be optional
.
And in the spec file ,the registrat should be alt in the picture, and user can setting the client ID by himself which we provide this interface ,
There was a problem hiding this comment.
GitHub, for example, does not provide a value for registration_endpoint in their response, as they do not support DCR and require pre-registration of clients. (I had to make this same change in a fork, to support GitHub properly, and had not yet had an opportunity to open a PR here.)
There was a problem hiding this comment.
Pull Request Overview
This PR makes dynamic client registration optional in the OAuth implementation, aligning with RFC 8414 standards. The change allows systems to function without supporting dynamic registration by providing appropriate fallback behavior.
- Changed
registration_endpointfrom requiredStringtoOption<String> - Added graceful fallback when dynamic registration is not supported
- Improved logging to use warnings instead of errors for expected fallback scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
examples/servers/src/complex_auth_sse.rs |
Wraps registration endpoint in Some() to match new optional type |
crates/rmcp/src/transport/auth.rs |
Updates struct definition, fallback logic, and error handling for optional registration |
Comments suppressed due to low confidence (2)
crates/rmcp/src/transport/auth.rs:398
- Removed error logging for HTTP status failures. Registration failures with specific HTTP status codes should be logged to help diagnose server-side issues.
return Err(AuthError::RegistrationFailed(format!(
"HTTP {}: {}",
status, error_text
)));
crates/rmcp/src/transport/auth.rs:408
- Removed error logging for JSON parsing failures. When the server returns an unparseable response, this should be logged as it indicates a protocol violation or server issue.
Err(e) => {
return Err(AuthError::RegistrationFailed(format!(
"analyze response error: {}",
e
)));
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: jokemanfire <[email protected]>
|
@4t145 Did you forget submit your review? :） |
|
approved |
|
LGTM as well after adjusting commit message to satisfy the linter |
jokemanfire
merged commit 6cd779c
into
modelcontextprotocol:main
Change it while merge . |
4 participants
The dynamic registration can be skipped, and it should be optional.
Motivation and Context
Accoding to rfc8414.
How Has This Been Tested?
No test
Breaking Changes
Types of changes
Checklist
Additional context